Skip to content

workaround for signal during grpc test framework teardown - #1691

Merged
rapids-bot[bot] merged 3 commits into
NVIDIA:mainfrom
tmckayus:fix/grpc-teardown-ci-unblock
Aug 28, 2026
Merged

workaround for signal during grpc test framework teardown#1691
rapids-bot[bot] merged 3 commits into
NVIDIA:mainfrom
tmckayus:fix/grpc-teardown-ci-unblock

Conversation

@tmckayus

@tmckayus tmckayus commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Tests themselves pass, but something in the teardown of the test framework does a double free. Unblock CI while we search for the root cause.

Best guess is something in the gRPC/abseil stack. Will investigate separately.

Tests themselves pass, but something in the teardown of the
test framework does a double free. Unblock CI while we
search for the root cause.
@tmckayus
tmckayus requested review from a team as code owners August 7, 2026 15:12
@tmckayus tmckayus added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Aug 7, 2026
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 769a77d8-20db-4836-9dbc-ff6d616378be

📥 Commits

Reviewing files that changed from the base of the PR and between 850beea and 142351e.

📒 Files selected for processing (1)
  • cpp/tests/linear_programming/grpc/grpc_integration_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • cpp/tests/linear_programming/grpc/grpc_integration_test.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The gRPC integration test now captures the GoogleTest result and uses std::_Exit(rc) to bypass static and atexit destructor teardown.

Changes

Post-teardown crash handling

Layer / File(s) Summary
Test process exit behavior
cpp/tests/linear_programming/grpc/grpc_integration_test.cpp
main stores the result from RUN_ALL_TESTS() and exits with std::_Exit(rc) instead of returning directly.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 14235

This localized test-only change is merge-ready after normal checks and review; no actionable merge-blocking risk remains.

Suggested reviewers: gforsyth, bubullzz, nguidotti

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the teardown signal workaround for the gRPC test framework. It accurately summarizes the main change.
Description check ✅ Passed The description explains the CI failure, the suspected double free during teardown, and the purpose of the workaround. It directly relates to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@ci/utils/junit_helpers.py`:
- Around line 54-60: Update xml_all_passed with the requested type annotations,
using xml_path: str and -> bool. Expand its docstring to document the xml_path
parameter, the conditions for returning True, and that file or XML parsing
errors return False.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 10b14469-f02d-4d95-95d8-6683c1ee57ee

📥 Commits

Reviewing files that changed from the base of the PR and between ae0a38a and 1bf2db5.

📒 Files selected for processing (3)
  • ci/run_ctests.sh
  • ci/utils/junit_helpers.py
  • cpp/tests/linear_programming/grpc/grpc_integration_test.cpp

Comment thread ci/utils/junit_helpers.py Outdated
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

CI Test Summary

✅ All 31 test job(s) passed.

@github-actions

Copy link
Copy Markdown

🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you!

If this is an "epic" issue, then please add the "epic" label to this issue.
If it is a PR and not ready for review, then please convert this to draft.
If you just want to switch off this notification, then use the "skip inactivity reminder" label.

@ramakrishnap-nv

Copy link
Copy Markdown
Collaborator

I traced this through the nightly path. Short version: it won't break nightly, but the shell-side heuristic opens a false-green hole that's wider than the bug it works around.

Checked and fine

  • --gtest_list_tests piping. This was my main worry: run_ctests.sh:130 pipes the binary's stdout into junit_helpers.py gtest-list in the nightly crash path, and piped stdout is block-buffered — std::_Exit skipping the stdio flush would have emptied that listing and turned every nightly crash-retry into FAILED: Could not list tests. It doesn't, because ListTestsMatchingFilter ends with an explicit fflush(stdout) (googletest gtest.cc:6426).
  • Log tail and XML survive _Exit. PrettyUnitTestResultPrinter::OnTestIterationEnd ends in fflush(stdout) (gtest.cc:3843) and XmlUnitTestResultPrinter::OnTestIterationEnd does fclose(xmlout) (gtest.cc:4093). Nothing is left buffered.
  • Coverage. No gcov on C++ (test.yaml:62 sets run_codecov: false; coverage is Python-only), so the usual "_Exit drops coverage counters" objection doesn't apply.
  • Orphaned servers / port leaks. Your safety claim holds — every suite's TearDownTestSuite does s_server_.reset(), which runs ~ServerProcessstop()kill(-pgid_, ...), all inside RUN_ALL_TESTS.
  • compute-sanitizer. test_cpp_memcheck.sh:60 only runs ROUTING_TEST/ROUTING_GES_TEST, so this binary never goes through it.

The concern

accept_post_pass_teardown_crash lives in run_gtest_with_retry, which loops over all "${GTEST_DIR}"/*_TEST (run_ctests.sh:215). But this binary now _Exits and can never reach that path — so the heuristic exists purely to swallow signal-deaths in other binaries, which is exactly where we have no diagnosed root cause.

The masked window is narrower than I first assumed, to be fair: global Environment::TearDown() runs at gtest.cc:6119, before the XML write at 6141, so a crash in a gtest global environment still produces no XML and is still caught. What's masked is strictly "after gtest wrote XML" — tail of UnitTest::Run, return from main, and static/atexit destructors. That's still precisely where a genuine libcuopt/RMM/CUDA static-destruction-order bug would surface, and it's user-facing: anything linking libcuopt would abort at exit the same way.

It also leaves no telemetry. nightly_report.py:130 collects results via rglob("*.xml") only — it never reads the run log, so the WARNING: line reaches neither the report, the history file, nor the dashboard. Between that and _Exit, the grpc double-free now produces zero signal, which makes "investigate separately" start with no frequency data on whether it's getting better or worse.

Suggestion

Keep std::_Exit(rc) — it's targeted and verifiably safe for this binary. For the shell heuristic, either:

  1. Scope it to this binary (name check or an env var set only for GRPC_INTEGRATION_TEST), so an exit-time abort anywhere else still fails nightly; or
  2. If it stays global, have it write a *-teardown-crash.xml marker via the existing write_crash_xml in crash_helpers.sh as a passing-but-recorded entry, so nightly_report.py can track the occurrence rate instead of the signal vanishing.

Minor

  • The new block under [ -z "${tests_to_retry}" ] is unreachable-when-true: getting there requires a signal death, and accept_post_pass_teardown_crash already evaluated the identical xml_reports_all_passed condition and returned 0. Harmless, but it reads like a second safety net that can never fire.
  • std::_Exit is declared in <cstdlib>, which grpc_integration_test.cpp doesn't include (it has <cerrno>, <cmath>, <filesystem>, <fstream>, <mutex>, <sstream>). It compiles today only transitively — worth adding explicitly.
  • xml_all_passed counts a <skipped/> testcase as a pass. Probably fine here, just noting it.

@ramakrishnap-nv ramakrishnap-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmckayus unblocking with a PR. but please take a look at the concerns above and coderabbit suggestion.

@github-actions

Copy link
Copy Markdown

🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you!

If this is an "epic" issue, then please add the "epic" label to this issue.
If it is a PR and not ready for review, then please convert this to draft.
If you just want to switch off this notification, then use the "skip inactivity reminder" label.

std::_Exit already skips the gRPC/Abseil static destructor abort in
GRPC_INTEGRATION_TEST; treating signal deaths as passes for every
*_TEST binary would hide real exit-time failures.

Signed-off-by: Trevor McKay <tmgithub1@gmail.com>
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@tmckayus

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit 269fb4c into NVIDIA:main Aug 28, 2026
201 of 205 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants